-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(c/driver/postgresql): Implement consuming a PGresult via the copy reader #2029
feat(c/driver/postgresql): Implement consuming a PGresult via the copy reader #2029
Conversation
a375c63
to
63b9459
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is great!
return -1; | ||
} | ||
|
||
char* first = PQcmdTuples(result_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...wow, this function has such a weird API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is what you thought the last time you encountered it, too 🙂
arrow-adbc/c/driver/postgresql/statement.cc
Lines 1352 to 1353 in 45cd9be
// an empty string even for a DELETE. (Also, why does it return a | |
// string...) Possibly, it doesn't work because we use PQexecPrepared |
NANOARROW_RETURN_NOT_OK(field_readers_[i]->InitArray(tmp->children[i])); | ||
} | ||
|
||
// TODO: If we get an EOVERFLOW here (e.g., big string data), we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we file a new issue for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely! #2064
643423d
to
353100f
Compare
I started this PR wanting to get queries with parameters able to return their results; however, this turned into a PR leaning in to the
PqResultHelper
because it was helpful to export arrays from thePGresult*
but wasn't quite general enough. I did a second bit of shuffling to make it (possibly, or maybe just for me) easier to understand what path gets taken onExecuteQuery()
.Some side effects of these changes are that we can now support multiple statements in the same query (by using
PQexec()
instead ofPQexecParams()
when there is no output requested) and that we canExecuteSchema()
for all parameterized queries.The actual feature is that a user can set
adbc.postgresql.use_copy = FALSE
to force a non-COPY path for queries that aren't supported there. Because we request binary data, we can use all the same infrastructure for converting the results! I have only one test for this although I did run the whole test suite in C++ and Python...there are still a few missing features (batch size hint, large string overflow, error detail, cancel) but most tests pass using either path.I'm happy to split this up if that is easier! I'm also planning to document the helper (but wanted a first round of review before documenting the behaviour to make sure it's behaviour we actually want).
Closes #855, Closes #2035.
Created on 2024-07-25 with reprex v2.1.0